Skip to content

Add Decimal32, Decimal64, Decimal128#100729

Open
RaymondHuy wants to merge 126 commits into
dotnet:mainfrom
RaymondHuy:issue-81376
Open

Add Decimal32, Decimal64, Decimal128#100729
RaymondHuy wants to merge 126 commits into
dotnet:mainfrom
RaymondHuy:issue-81376

Conversation

@RaymondHuy

Copy link
Copy Markdown
Contributor

Resolve #81376

@ghost

ghost commented Apr 6, 2024

Copy link
Copy Markdown

Note regarding the new-api-needs-documentation label:

This serves as a reminder for when your PR is modifying a ref *.cs file and adding/modifying public APIs, please make sure the API implementation in the src *.cs file is documented with triple slash comments, so the PR reviewers can sign off that change.

@RaymondHuy RaymondHuy marked this pull request as draft April 6, 2024 17:29
@dotnet-policy-service dotnet-policy-service Bot added the community-contribution Indicates that the PR has been added by a community member label Apr 6, 2024
Comment thread src/libraries/System.Private.CoreLib/src/System/Number.Parsing.cs Outdated
Comment thread src/libraries/System.Private.CoreLib/src/System/Numerics/Decimal128.cs Outdated
Comment thread src/libraries/System.Private.CoreLib/src/System/Numerics/Decimal128.cs Outdated
Comment thread src/libraries/System.Private.CoreLib/src/System/Number.DecimalIeee754.cs Outdated
@tannergooding

Copy link
Copy Markdown
Member

This is on my radar to take a look at; just noting it might be a bit delayed due to other priorities for the .NET 9 release.

CC. @jeffhandley

@RaymondHuy RaymondHuy marked this pull request as ready for review April 9, 2024 17:21
Comment thread src/libraries/System.Private.CoreLib/src/System/Number.DecimalIeee754.cs Outdated
Comment thread src/libraries/System.Private.CoreLib/src/System/Number.DecimalIeee754.cs Outdated
@PranavSenthilnathan

Copy link
Copy Markdown
Member

@RaymondHuy can you add test cases for the last few cases you fixed:

  • CompareTo/GetHashCode of different infinities/NaN
  • Parsing 0.500000000001x10^-101 (parse buffer only stores 0.50000000x10^-101 but has non-zero tail)
  • The counterexample you found for why normalizedCurrentSignificand approach doesn't work for comparisons

I'm good with the PR otherwise and can approve with the added tests.

@RaymondHuy

Copy link
Copy Markdown
Contributor Author

@tannergooding

Copy link
Copy Markdown
Member

@RaymondHuy looks like there's still a couple test failures:

System.Tests.Decimal128Tests.CompareTo_Other_ReturnsExpected
System.Tests.Decimal64Tests.CompareTo_Other_ReturnsExpected

Both are comparing zero (0.000.....0 vs 0.000....1) and give stack traces are similar to:

at System.UInt64.DivRem(UInt64 left, UInt64 right)
   at System.Number.<CompareDecimalIeee754>g__InternalUnsignedCompare|3_0[TDecimal,TValue](DecodedDecimalIeee754`1 current, DecodedDecimalIeee754`1 other)
   at System.Number.CompareDecimalIeee754[TDecimal,TValue](TValue currentDecimalBits, TValue otherDecimalBits)
   at System.Numerics.Decimal64.CompareTo(Decimal64 other)
   at System.Tests.Decimal64Tests.CompareTo_Other_ReturnsExpected(Decimal64 d1, Decimal64 d2, Int32 expected)
   at InvokeStub_Decimal64Tests.CompareTo_Other_ReturnsExpected(Object, Span`1)
   at System.Reflection.MethodBaseInvoker.InvokeWithFewArgs(Object obj, BindingFlags invokeAttr, Binder binder, Object[] parameters, CultureInfo culture)

Comment on lines +176 to +180
int diffExponent = current.UnbiasedExponent - other.UnbiasedExponent;
if (diffExponent < TDecimal.Precision)
{
TValue factor = TDecimal.Power10(diffExponent);
(TValue quotient, TValue remainder) = TValue.DivRem(other.Significand, current.Significand);

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@RaymondHuy the issue looks to be here and I expect it will still throw for yield return new object[] { Decimal128.Parse("0.5" + new string('0', 300) + "1e-6178"), Decimal128.Epsilon, -1 }; (using -1 as the expected result because the string parses to 0 which is less than Epsilon)

In that case, the exponent will differ but the current.Significand is zero

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for pointing out. My mistake, let me update it. :(

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please make sure to add in the relevant test since as well, thanks!

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi @tannergooding I have fixed the DivideByZeroException in this commit: 5f534ec

added test case in this commit bae1c3b

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area-System.Numerics community-contribution Indicates that the PR has been added by a community member new-api-needs-documentation

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[API Proposal]: Add Decimal32, Decimal64, and Decimal128 from the IEEE 754-2019 standard.

10 participants